Refactor handling of TOML config files for runtimes #643
Conversation
elezar
force-pushed
the
refactor-toml-source
branch
from
0fa4e8b to
1863c1b
Compare
pkg/config/source.go
Outdated
| return LoadBytes(outb.Bytes()) | ||
| } | ||
|
|
||
| type tomlFromCRI struct { |
There was a problem hiding this comment.
Given that crictl info doesn't provide the full config for CRI-O, this is probably not a useful toml source candidate
There was a problem hiding this comment.
We should probably just stick to the file-based and CLI-based sources for now IMO
There was a problem hiding this comment.
Sure. I've removed the logic from this PR though. We can add it as a follow-up.
elezar
force-pushed
the
refactor-toml-source
branch
2 times, most recently
from
5e45d7f to
f3c8ea0
Compare
elezar
changed the title
Signed-off-by: Evan Lezar <elezar@nvidia.com>
elezar
force-pushed
the
refactor-toml-source
branch
from
f3c8ea0 to
aa8ec43
Compare
| @@ -14,19 +14,19 @@ | |||
| # limitations under the License. | |||
| **/ | |||
|
|
|||
| package engine | |||
| package config | |||
There was a problem hiding this comment.
Why has this been moved from engine to config
There was a problem hiding this comment.
Because this represents a Raw config. Importing enging.Raw does not make the intent as clear as config.Raw when imported.
pkg/config/engine/crio/option.go
Outdated
| logger logger.Interface | ||
| path string | ||
| logger logger.Interface | ||
| reference toml.Loader |
There was a problem hiding this comment.
I feel like the name reference may be a bit too vague. How about configRef?
There was a problem hiding this comment.
I changed this to configSource instead. One thing I would like to do as a (non-critical) follow-up is to also refactor the docker config handling to make use of a source like this. Not a blocker for the CRI-O work though.
| } | ||
|
|
||
| if os.IsNotExist(err) { | ||
| return Empty.Load() |
There was a problem hiding this comment.
I think we should fail fast here and error out indicating that the file wasn't found
There was a problem hiding this comment.
That is not how the current code works. We load an empty config if the file doesn't exist.
Can you think of a reason to fail instead? How would a user indicate that the config is optional?
There was a problem hiding this comment.
A path to a file that does not exist should be considered as invalid input IMO. So we should error out accordingly
tools/container/crio/crio.go
Outdated
| @@ -222,6 +223,7 @@ func setupConfig(o *options) error { | |||
|
|
|||
| cfg, err := crio.New( | |||
| crio.WithPath(o.Config), | |||
| crio.WithReference(toml.FromFile(o.Config)), | |||
There was a problem hiding this comment.
I think WithConfigReference may be more suggestive of the struct field's role and purpose
There was a problem hiding this comment.
I think WithConfigSource is clearer. Have updated.
elezar
force-pushed
the
refactor-toml-source
branch
from
aa8ec43 to
033aa5c
Compare
This change refactors the toml config file handlig for runtimes such as containerd or crio. A toml.Loader is introduced that encapsulates loading the required file. This can be extended to allow other mechanisms for loading loading the current config. Signed-off-by: Evan Lezar <elezar@nvidia.com>
elezar
force-pushed
the
refactor-toml-source
branch
from
033aa5c to
bf2bdfd
Compare
This change refactors the toml config file handlig for runtimes
such as containerd or crio. A toml.Loader is introduced that
encapsulates loading the required file.
This can be extended to allow other mechanisms for loading
loading the current config.